Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-work logging and command-line argument parsing #3980

Merged
merged 10 commits into from
Jun 16, 2023

Conversation

Michael-F-Bryan
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan commented Jun 9, 2023

When discussing #3948, we decided that the wasmer command shouldn't show any log messages unless the user explicitly sets the $RUST_LOG environment variable. That way there's no chance that logging will mess up the output you might get from a program.

I've also re-worked the CLI parsing code to be vanilla clap rather than having a mix of manual argument parsing and two different clap parsers. This means we no longer need to update that big match statement every time a new command or alias is added, plus it makes the code a lot more readable.

Fixes #3948.

lib/cli/src/commands/run.rs Show resolved Hide resolved
lib/cli/src/cli.rs Outdated Show resolved Hide resolved
@syrusakbary syrusakbary added this to the v4.0 milestone Jun 13, 2023
@Michael-F-Bryan Michael-F-Bryan changed the title Disable logging unless the $RUST_LOG variable is set Re-work logging and command-line argument parsing Jun 15, 2023
@Michael-F-Bryan Michael-F-Bryan force-pushed the disable-logging-by-default branch from 7fe964d to 4f0438c Compare June 15, 2023 09:22
Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

Love getting rid of the command hacks!

@Michael-F-Bryan Michael-F-Bryan force-pushed the disable-logging-by-default branch from ded7c54 to c923745 Compare June 15, 2023 10:14
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but can we have a test testing that wasmer awasifile.wasm still works before merging? Thanks!

@Michael-F-Bryan
Copy link
Contributor Author

@syrusakbary most of the wasmer run integration tests already check wasmer run whatever.wasm and wasmer whatever.wasm, so we should be all good on that front 👌

For example,

// test with "wasmer qjs.wasm"
Command::new(get_wasmer_path())
.arg(temp_dir.path())
.arg("--")
.arg("--quit")
.assert()
.success();
// test again with "wasmer run qjs.wasm"
Command::new(get_wasmer_path())
.arg("run")
.arg(temp_dir.path())
.arg("--")
.arg("--quit")
.assert()
.success();

@Michael-F-Bryan Michael-F-Bryan force-pushed the disable-logging-by-default branch from c923745 to 4a0b438 Compare June 15, 2023 11:05
@Michael-F-Bryan Michael-F-Bryan force-pushed the disable-logging-by-default branch 2 times, most recently from 4b7c86d to e3abe19 Compare June 16, 2023 13:51
@Michael-F-Bryan Michael-F-Bryan force-pushed the disable-logging-by-default branch from e3abe19 to e620278 Compare June 16, 2023 15:07
@Michael-F-Bryan Michael-F-Bryan merged commit 568a214 into master Jun 16, 2023
@Michael-F-Bryan Michael-F-Bryan deleted the disable-logging-by-default branch June 16, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Showing unnecessary WARN when running a package
3 participants